Skip to content

Conversation

@hayemaxi
Copy link
Contributor

Follow up to #1176 (comment)

  • Seamlessly de-duplicate hook names in a single config instead of making the user deal with it
    • Also checks the config file and deduplicates any names in case they were added outside of chat
  • Use separate hook caches for profile vs global hooks:
    • Allows hooks to have the same names across profile/global configs
    • Allows us to clear execute cache easily when switching profiles

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@hayemaxi hayemaxi requested a review from a team April 13, 2025 20:11
@codecov-commenter
Copy link

codecov-commenter commented Apr 13, 2025

Codecov Report

Attention: Patch coverage is 11.38211% with 109 lines in your changes missing coverage. Please review.

Project coverage is 14.03%. Comparing base (82d91ff) to head (80dec01).
Report is 200 commits behind head on main.

Files with missing lines Patch % Lines
crates/q_cli/src/cli/chat/mod.rs 3.07% 62 Missing and 1 partial ⚠️
crates/q_cli/src/cli/chat/hooks.rs 8.00% 23 Missing ⚠️
crates/q_cli/src/cli/chat/context.rs 26.66% 21 Missing and 1 partial ⚠️
crates/q_cli/src/cli/chat/command.rs 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1216      +/-   ##
==========================================
- Coverage   14.04%   14.03%   -0.01%     
==========================================
  Files        2366     2366              
  Lines      205444   205443       -1     
  Branches   185808   185807       -1     
==========================================
- Hits        28845    28841       -4     
- Misses     175175   175178       +3     
  Partials     1424     1424              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hayemaxi hayemaxi force-pushed the context-hooks-final branch 2 times, most recently from 1c0ed4e to 7e29f10 Compare April 14, 2025 14:46
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like having a lot of extra logic for validating and creating new hook names when this could also be solved by just changing the structure used when saving hooks. We have that ContextConfig is already saved per profile, so instead of saving hooks as a list of hooks, why not instead have it be:

ContextConfig {
    paths: ...,
    hooks: HookConfig {
        conversation_start: { [key: string]: Hook },
        per_prompt: { [key: string]: Hook },
    }
}

This way we could also avoid having to create randomly generated id's for hook names. I feel it's simpler for us and the user to just present an error to the user stating a hook with that name already exists instead of this approach.

Copy link
Contributor Author

@hayemaxi hayemaxi Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ContextConfig {
    paths: ...,
    hooks: HookConfig {
        conversation_start: { [key: string]: Hook },
        per_prompt: { [key: string]: Hook },
    }
}

This won't work either. Hook names have to be unique across conversation_start AND per_prompt, otherwise this is ambiguous: /context hooks rm my_hook. my_hook could be in either of the categories.

This way we could also avoid having to create randomly generated id's for hook names. I feel it's simpler for us and the user to just present an error to the user stating a hook with that name already exists instead of this approach.

It's not random, and I would argue its easier for the user because there is never a failure point for them. Their hook always gets added. Also, there is a lack of any config validation happening that we desperately need, whether right now or later (currently if there is an issue with the JSON then the context manager just doesn't load without any warning to the user).
However, the automatic renaming may be confusing, and maybe this issue is a just a symptom of the structure of the config. Maybe we can resolve this by just changing the schema to not have these separations. Thoughts?

context.json

{
    paths: [ ... ],
    hooks: {
        my_hook : { // the name, which is now unique amongst all hooks in the config file
             // category
             "when": "conversation_start" | "per_prompt", // open to a different name, could be "on" as well
             
              ... normal hook data
        }
        ...
    }
}

Copy link
Contributor

@brandonskiser brandonskiser Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and I would argue its easier for the user because there is never a failure point for them. Their hook always gets added. Also, there is a lack of any config validation happening that we desperately need, whether right now or later

Definitely true about handling validation and serialization errors - although, the random naming is still just strange to me because hooks should probably have some more semantic meaning to their name instead of just having the user get away with hook_1, hook_2, hook_3 etc. I'd like to hear others thoughts on this hook naming part before going with this approach.

Maybe we can resolve this by just changing the schema to not have these separations. Thoughts?

I'd prefer this! Saves having to have hook name as a validation check in any validation logic we might have.

Updated the schema to be:
```
{
  "hooks": {
    "my_hook_name": {
      ...hook data
      "trigger": "conversation_start" | "per_prompt",
    }
  }
}
```
So that we do not have to worry about duplicate name hooks or generating an ID for a hook. Whether the user adds a new hook via commands or editing the JSON file, the invariant holds true.

Additionally:
- Separate execution caches for profile/global, so that we don't have to worry about name overlappying
  - also allows us to clear hook cache when switch profile
- Rename CLI arg `--type` to `--trigger` for `/context hooks add ...`
@hayemaxi hayemaxi force-pushed the context-hooks-final branch from 7e29f10 to 80dec01 Compare April 14, 2025 21:37
@hayemaxi hayemaxi merged commit 6363716 into aws:main Apr 14, 2025
10 checks passed
jsamuel1 pushed a commit that referenced this pull request Apr 15, 2025
Updated the schema to be:
```
{
  "hooks": {
    "my_hook_name": {
      ...hook data
      "trigger": "conversation_start" | "per_prompt",
    }
  }
}
```
So that we do not have to worry about duplicate name hooks or generating an ID for a hook. Whether the user adds a new hook via commands or editing the JSON file, the invariant holds true.

Additionally:
- Separate execution caches for profile/global, so that we don't have to worry about name overlappying
  - also allows us to clear hook cache when switch profile
- Rename CLI arg `--type` to `--trigger` for `/context hooks add ...`
This was referenced Apr 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants